Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Accept 0 as a valid value for EventListener replicas #925

Closed
wants to merge 1 commit into from
Closed

Accept 0 as a valid value for EventListener replicas #925

wants to merge 1 commit into from

Conversation

Fabian-K
Copy link
Contributor

Changes

Accept 0 as a valid value for EventListener replicas. This change is intended as a workaround until a proper Knative EventListener implementation is available (see #892). Note: without additional work, replicas=0 results in a broken setup.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes tests (if functionality changed/added)
  • Includes docs (if user facing)
  • Commit messages follow commit message best practices
  • Release notes block has been filled in or deleted (only if no user facing changes)

See the contribution guide for more details.

Release Notes

Accept 0 as a valid value for EventListener replicas. Previously, this was changed to 1. Note: without additional work, replicas=0 results in a broken setup.

@Fabian-K
Accept 0 as a valid value for EventListener replicas
53523dd 
This change is intended as a workaround until a proper Knative EventListener implementation is available (see tektoncd#892). Note: without additional work, replicas=0 results in a broken setup.
@tekton-robot tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jan 25, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jan 25, 2021 •
edited
Loading

CLA Signed

The committers are authorized under a signed CLA.

@tekton-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dlorenc
You can assign the PR to them by writing /assign @dlorenc in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@tekton-robot tekton-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 25, 2021
@tekton-robot
Copy link

Hi @Fabian-K. Thanks for your PR.

I'm waiting for a tektoncd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@MarcelMue
Copy link
Member

/ok-to-test

@tekton-robot tekton-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 25, 2021
@MarcelMue
Copy link
Member

Do you have any opinions here @savitaashture @dibyom ? It seems fine to me.

@savitaashture
Copy link
Contributor

@Fabian-K shall we do this change once we have changes related to TEP-0008 is in
Because i feel changing it now don't make any difference.

@Fabian-K
Copy link
Contributor Author

@savitaashture the overall motivation for this is to achieve scaling to zero. Once TEP-0008 is implemented and Tekton Triggers can directly generate a Knative Service instead of the Deployment this is not required anymore.

Until this is the case, this PR should allow by specifying replicas = 0 to get rid of the always running event listener pod. Of course, this leads to a broken setup but this can be fixed by manually generating e.g. a Knative Service as outlined in #892 (comment)

If the implementation of TEP-0008 is almost finished, it does not make sense to merge this. If this still requires time, this at least enables a workaround to get it faster.

@savitaashture
Copy link
Contributor

Here is the issue for TEP-0008
Planning to finish it in 0.12 Trigger release

@dibyom
Copy link
Member

dibyom commented Jan 27, 2021

If the implementation of TEP-0008 is almost finished, it does not make sense to merge this. If this still requires time, this at least enables a workaround to get it faster.

This sounds good to me.

@savitaashture are you planning on implementing Knative EL for v0.12? In that case maybe we can hold this PR; otherwise we can merge.

@savitaashture
Copy link
Contributor

If the implementation of TEP-0008 is almost finished, it does not make sense to merge this. If this still requires time, this at least enables a workaround to get it faster.

This sounds good to me.

@savitaashture are you planning on implementing Knative EL for v0.12? In that case maybe we can hold this PR; otherwise we can merge.

@dibyom Yes i am planning this for v0.12

But i want to understand the priority from @Fabian-K and if they need it immediately lets merge because anyhow in future we are going to remove this validation.

@Fabian-K
Copy link
Contributor Author

There is no immediate need, still it would be nice because we could potentially get rid of ~350 permanently running eventlinstener pods :) I don´t know the timeline for 0.12, let´s say if you plan to release that within the next 1-2 months I´m good with discarding this PR

@savitaashture savitaashture mentioned this pull request Feb 17, 2021
Merged
4 tasks
@Fabian-K
Copy link
Contributor Author

Fabian-K commented Mar 3, 2021

this is not required any more 🥳

@Fabian-K Fabian-K closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iancoffey iancoffey Awaiting requested review from iancoffey

@ncskier ncskier Awaiting requested review from ncskier

Assignees
No one assigned
Labels
ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Fabian-K @tekton-robot @MarcelMue @savitaashture @dibyom